Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementing password strength check #1071

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

ngn13
Copy link
Contributor

@ngn13 ngn13 commented Jun 14, 2023

I removed:

minLength: number;
minNumbers: number;
minUpperCase: number
minSymbols: number

and instead implemented

strength: number;

Also I removed the shannon entropy calculation, I couldn't get it to work and idk math
entropy calculation now works thanks to @greysilly7

Copy link
Contributor

@erkinalp erkinalp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am against this PR
the entropy calculation has an information theoretical basis here, and it checks how much information that the password carries relative to an incompressible string (which is what we want an ideal password to be, not an unrememberable string of special characters)

@@ -41,25 +41,25 @@ export function checkPassword(password: string): number {
let pwStrength = 0;

// checks for total password len
if (password.length >= 8 - 1) {
if (password.length >= 7) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these might been hard coded values for min and max that should have been configurable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should I add the configuration options back then?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should I add the configuration options back then?

So far, we've tried to make as much as possible configurable my the instance owner, and I think the password policy would be no exception. But probably wait for other opinions on it.

@ngn13
Copy link
Contributor Author

ngn13 commented Jun 14, 2023

I am against this PR the entropy calculation has an information theoretical basis here, and it checks how much information that the password carries relative to an incompressible string (which is what we want an ideal password to be, not an unrememberable string of special characters)

I made a mistake with my wording, I didn't exactly remove it. See the "Files changed".

function calculateEntropy(str: string) {
	// TODO: calculate the shannon entropy
	return 0;
}

@erkinalp
Copy link
Contributor

@ngn13 Your change makes the entropy calculation always return zero, which is not the desired behaviour here.

@ngn13
Copy link
Contributor Author

ngn13 commented Dec 26, 2023

I understand that, the previous implementation did not work properly (and idk how to fix it), so I commented it out and left a to-do note so someone who knows math can implement it later.

Signed-off-by: Scott Gould <[email protected]>
@greysilly7
Copy link
Contributor

greysilly7 commented Jan 1, 2024

This will most likely work ngn13#1 I don't have the environment to test it right now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants